Skip to content

Make site work with the Cloudflare OpenNext adapter #7383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Jan 2, 2025

This PR applies changes to make it so that the site can be deployed to Cloudflare workers using the open-next Cloudflare adapter

The app does seem to work as intended for the most part:
Screenshot 2025-01-02 at 19 46 00

Deployment URL: https://nodejs-website.web-experiments.workers.dev


Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Copy link

vercel bot commented Jan 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 12, 2025 3:22pm

@ovflowd
Copy link
Member

ovflowd commented Feb 1, 2025

@dario-piotrowicz do we have updates here? 👀

@dario-piotrowicz
Copy link
Member Author

Hey @ovflowd 👋

Sorry for keeping the PR lingering, there are a few smaller issues we addressed in our adapter (that I need to reflect here), and also ISR should be coming soon (@vicb can provide more context)

Besides that I just need to rebase the PR, the only significant issue remaining should be filesystem access, but I wanted to clarify that with you, I'll drop you a message today to clarify things

(PS: I hope the PR is not bothering you 🙇, if you want I can close it and reopen it when we're ready?)

@ovflowd
Copy link
Member

ovflowd commented Feb 16, 2025

Sorry for keeping the PR lingering, there are a few smaller issues we addressed in our adapter (that I need to reflect here), and also ISR should be coming soon (@vicb can provide more context)

This is awesome news. Excited to hear from @vicb

Besides that I just need to rebase the PR, the only significant issue remaining should be filesystem access, but I wanted to clarify that with you, I'll drop you a message today to clarify things

I believe we sorted that out on Slack 🖖

(PS: I hope the PR is not bothering you 🙇, if you want I can close it and reopen it when we're ready?)

Not at all <3

@avivkeller
Copy link
Member

I've resolved #7383 (comment). With that in mind, I'd love to merge this tomorrow (or sooner, if you'd like) if there are no objections1. WDYT?

Footnotes

  1. Re-open that discussion if you feel it's not resolved.

update the site application so that it can be build using the
Cloudflare OpenNext adapter (`@opennextjs/cloudflare`) and thus
deployed on Cloudflare Workers
@dario-piotrowicz
Copy link
Member Author

I've resolved #7383 (comment). With that in mind, I'd love to merge this tomorrow (or sooner, if you'd like) if there are no objections1. WDYT?

Thank you very much @avivkeller 🫶

With that resolved I think this should (finally! sorry about how long this took! 😅) be ready to be merged! 😄 🥳

I'm also happy to merge this as long as there are no objections 😃

@avivkeller
Copy link
Member

It's your PR, so feel free to merge whenever you'd like. Honestly, Any changes could be done in a follow up.

@dario-piotrowicz dario-piotrowicz added this pull request to the merge queue May 12, 2025
Merged via the queue into nodejs:main with commit aef4d8f May 12, 2025
14 of 16 checks passed
@dario-piotrowicz dario-piotrowicz deleted the dario/opennextjs-cloudflare branch May 12, 2025 15:52
@dario-piotrowicz
Copy link
Member Author

Finally made it! thanks a lot everyone for the reviews and the support on this one! 🫶

@ovflowd
Copy link
Member

ovflowd commented May 12, 2025

This PR worries me; It got 0 approvals from web-infra when it is pretty much an infrastructure PR. Ideally this should have gotten an approval from at least me, @bmuenzenmeyer or @flakey5 or @MattIPv4 or @canerakdas. But it got none :/

@ovflowd
Copy link
Member

ovflowd commented May 12, 2025

Anyhow, I believe my concerns can be addressed in a follow-up PR; I'd just really appreciate if they get done; I wouldn't mind doing'em myself to be honest.

@avivkeller
Copy link
Member

avivkeller commented May 12, 2025

That's on me. I encouraged the merging of this PR - and should've checked that web-infra got a chance to review.

Which of your concerns weren't addressed? I think the postinstall concern is resolved (Postinstall scripts are no longer in use).

@ovflowd
Copy link
Member

ovflowd commented May 12, 2025

Which of your concerns weren't addressed? I think the postinstall concern is resolved (Postinstall scripts are no longer in use).

That's OK; My concern was a base template file existing; Otherwise if I never run any of the run commands, or just do npx next dev it will just break. The run commands technically speaking should be as simple as possible.

I still wonder if we accidentally have too much complexity on the repository. I truly think we gotta audit the repository's complexity and attempt to trim it down. (Monorepo aside, cause that's fine)

@dario-piotrowicz
Copy link
Member Author

That's on me.

That's definitely on me as well (more than you I'd argue) 🥹 , sorry @ovflowd 🙇 , given your last approval (note that I addressed the comments that approval refers to) which is from a web-infra folk and the rest of the approvals I didn't really see a concern about the merge.

Yes I agree that this probably should have gotten some more web-infra visibility 🙇

@ovflowd
Copy link
Member

ovflowd commented May 12, 2025

That's on me.

That's definitely on me as well (more than you I'd argue) 🥹 , sorry @ovflowd 🙇 , given your last approval (note that I addressed the comments that approval refers to) which is from a web-infra folk and the rest of the approvals I didn't really see a concern about the merge.

Yes I agree that this probably should have gotten some more web-infra visibility 🙇

Indeed, I didn't realise I approved; That's on me, apologies for calling out there was no approval when there was indeed one. It's all good, and it's minor :)

Anyhow, I'm super proud of the work here ✨

@dario-piotrowicz
Copy link
Member Author

Indeed, I didn't realise I approved; That's on me, apologies for calling out there was no approval when there was indeed one. It's all good, and it's minor :)

Thanks! 🫶

(But again, sorry, I should have checked with you before merging 🙇)

Anyhow, I'm super proud of the work here ✨

Thanks, and thanks for all the help with landing this 🫶

@dario-piotrowicz
Copy link
Member Author

Anyhow, I believe my concerns can be addressed in a follow-up PR; I'd just really appreciate if they get done; I wouldn't mind doing'em myself to be honest.

That's OK; My concern was a base template file existing; Otherwise if I never run any of the run commands, or just do npx next dev it will just break. The run commands technically speaking should be as simple as possible.

@ovflowd as I mentioned in the conversation there I did try what you suggested but that didn't work, and on the top of my head I can't think of a way to make it work without a postinstall script, I'm happy to help here but I am not sure how, would you mind giving that a quick try on your end? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants